Extract uncurried functions from storage writes #6803
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6803 +/- ##
==========================================
+ Coverage 40.96% 40.98% +0.01%
==========================================
Files 2093 2096 +3
Lines 184478 184516 +38
==========================================
+ Hits 75577 75616 +39
- Misses 102579 102586 +7
+ Partials 6322 6314 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
zhangchiqing
force-pushed
the
leo/db-ops-writes
branch
from
0516981 to
8fa3022
Compare
zhangchiqing
requested review from
janezpodhostnik,
jordanschalm and
peterargue
| // Using these deprecated functions could minimize the changes during refactor and easier to review the changes. | ||
| // The simplified implementation of the functions are in the writes.go file, which are encouraged to be used instead. | ||
|
|
||
| func Upsert(key []byte, val interface{}) func(storage.Writer) error { |
There was a problem hiding this comment.
Originally these functions are implemented as curried functions in order to follow the same pattern as badger transaction.
However, since we are moving away from badger transaction, it's simpler just removing this additional wrap by using the uncurried functions.
I extracted the uncurried functions and kept the original functions here so that the same logic are reused, as well as the tests.
Useful for refactoring storage modules with UpsertKey instead of Upsert for instance.
utils/merr/closer.go
Outdated
| func CloseAndMergeError(closable io.Closer, err error) error { | ||
| var merr *multierror.Error | ||
| if err != nil { | ||
| merr = multierror.Append(merr, err) | ||
| } | ||
|
|
||
| closeError := closable.Close() | ||
| if closeError != nil { | ||
| merr = multierror.Append(merr, closeError) | ||
| } | ||
|
|
||
| return merr.ErrorOrNil() | ||
| } |
There was a problem hiding this comment.
| func CloseAndMergeError(closable io.Closer, err error) error { | |
| var merr *multierror.Error | |
| if err != nil { | |
| merr = multierror.Append(merr, err) | |
| } | |
| closeError := closable.Close() | |
| if closeError != nil { | |
| merr = multierror.Append(merr, closeError) | |
| } | |
| return merr.ErrorOrNil() | |
| } | |
| func CloseAndMergeError(closable io.Closer, original error) error { | |
| merr = multierror.Append(merr, closable.Close()) | |
| return merr.ErrorOrNil() | |
| } |
I'm not 100% but I think this will just work
There was a problem hiding this comment.
refactored and added test cases.
| "github.com/onflow/flow-go/model/flow" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
where did these constants come from? Is this a unrelated thing?
There was a problem hiding this comment.
zhangchiqing
force-pushed
the
leo/db-ops-writes
branch
from
8fa3022 to
a0068e1
Compare
| require.Nil(t, err) | ||
|
|
||
| // Test case 2: only original error | ||
| err = CloseAndMergeError(closer, errors.New("original error")) |
There was a problem hiding this comment.
You do test it further below when using storage.ErrNotFound, but it would be ideal to use assert.ErrorIs(...) for all of these test cases (instead of substring comparison of the error text with Contains).
errors.Is is what we use for sentinel error handling in the business logic, so it's better if the assertions use the same logic. If multierror didn't properly support error wrapping, or we weren't using it properly, substring comparisons could easily pass even if errors.Is would fail.
There was a problem hiding this comment.
I do have the errors.Is check below in Test case 5 and 6.
This case here is for exceptions where we don't have a sentinal error defined, and have to check with the error message.
utils/merr/closer.go
Outdated
| // // bad, because the definition of err might get overwritten | ||
| // err = closeAndMergeError(closable, err) |
There was a problem hiding this comment.
It can only be overwritten by another deferred function that is deferred before the function calling closeAndMergeError. It can't be overwritten by anything in the main function body.
I'm fine with the safety suggestions here, and appreciate the detailed documentation, but it does seem like a pretty small surface area. Maybe I'm missing something?
There was a problem hiding this comment.
I ran into a mistake that err was overwritten, but can't remember what was it now. I think I will keep this comment still basically says a different name is better than the default name.
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
Co-authored-by: Jordan Schalm <jordan.schalm@flowfoundation.org>
This PR: